Skip to content

Conversation

@sbomer
Copy link
Member

@sbomer sbomer commented Nov 21, 2024

The tag names should match the directory structure, and should include the architecture for arch-specific images.

The tag names should match the directory structure, and should include
the architecture for arch-specific images.
Copy link
Member

@mthalman mthalman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like these changes. It brings consistency and clarity. But I think we need to update the documentation to reflect this pattern because it is a different pattern than what is currently used for non-cross images.

I'm specifically referring to the pattern documented at https://github.com/dotnet/dotnet-buildtools-prereqs-docker?tab=readme-ov-file#how-to-identify-an-image. The difference with the cross images is that the architecture specified refers to the cross target, not the architecture of the image.

@sbomer
Copy link
Member Author

sbomer commented Nov 22, 2024

Thanks! The docs currently say this about the arch:

The tag format used by the images from this repository is mcr.microsoft.com/dotnet-buildtools/prereqs:<os-name>-<os-version>-<variant>-<architecture>
...
<architecture> - Architecture of the OS (amd64 shall be implied if not specified).

The folder structure used in src aligns with the tagging convention

If a Dockerfile is shared across multiple architectures, then the folder should be omitted.

Personally I like the policy of allowing -amd64 to be omitted from the tag format where it's implied, and keeping the simpler tags like azurelinux-3.0-net8.0-crossdeps-builder.

I propose we:

  • Allow omitting amd64 from the tag names and folder structure, and undo Move all Dockerfiles to arch-specific dir #1047 for images where we take advantage of this option.
  • Update docs to recommend the current pattern (-<target>) that we use for the cross-compilation images.

cc @richlander

@mthalman
Copy link
Member

mthalman commented Nov 22, 2024

Thanks! The docs currently say this about the arch:

The tag format used by the images from this repository is mcr.microsoft.com/dotnet-buildtools/prereqs:---
...
- Architecture of the OS (amd64 shall be implied if not specified).

The folder structure used in src aligns with the tagging convention

If a Dockerfile is shared across multiple architectures, then the folder should be omitted.

Personally I like the policy of allowing -amd64 to be omitted from the tag format where it's implied, and keeping the simpler tags like azurelinux-3.0-net8.0-crossdeps-builder.

I propose we:

  • Allow omitting amd64 from the tag names and folder structure, and undo Move all Dockerfiles to arch-specific dir #1047 for images where we take advantage of this option.
  • Update docs to recommend the current pattern (-<target>) that we use for the cross-compilation images.

cc @richlander

I don't like removing it from the directory name. Because there are some Dockerfiles which are truly multi-arch and so they have no architecture directory. Saying that an amd64 Dockerfile can get by without being contained in an amd64 directory causes confusion and ambiguity compared to the multi-arch ones.

For tags, I initially was ambivalent. But thinking more about it, I kind of feel it needs to be explicit because of the ambiguity that arises from it. One could imagine have multi-arch tags for our multi-arch Dockerfiles. And that would most definitely not have an arch in the tag name.

So I think the documented pattern should be changed to say that all architectures should be explicit both in tags and directories.

@sbomer
Copy link
Member Author

sbomer commented Nov 22, 2024

Thanks! Updated the readme to reflect this guidance and also mention the different pattern used for cross-arch images. I updated some of the alpine tags because the docs used alpine as an example.

sbomer and others added 2 commits November 22, 2024 13:44
Add examples for the target
@mthalman mthalman enabled auto-merge (squash) November 22, 2024 22:14
@mthalman mthalman merged commit bde3c9e into dotnet:main Nov 22, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants